Skip to content

fix(plugin-mysql): report the real error when a query fails while reading rows (#1884) - #1891

Merged
datlechin merged 2 commits into
mainfrom
fix/1884-mysql-order-by-empty-result
Jul 16, 2026
Merged

fix(plugin-mysql): report the real error when a query fails while reading rows (#1884)#1891
datlechin merged 2 commits into
mainfrom
fix/1884-mysql-order-by-empty-result

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #1884.

A MySQL SELECT ending in ORDER BY came back as an empty grid: no rows, no error, ~0.1s. The same query without the ORDER BY returned rows, and the reporter's other MySQL clients ran it fine.

Root cause

Two layers, both ours.

1. The silent failure (this is the bug). mysql_fetch_row() returns NULL for two different things: a clean end of rows, and an error part way through reading. The C API requires checking mysql_errno() to tell them apart. executeQuerySync only checked errno inside its if truncated branch, so when the loop ended naturally it returned whatever rows it had, [] if the failure hit before row one, as a successful query. Column metadata arrives before row data, so the result still had real column headers. That combination is exactly the report: correct-looking grid, zero rows, no error. The prepared-statement path had the same defect, treating MYSQL_NO_DATA (real end of rows) and a genuine error the same way.

ORDER BY exposes it because a filesort makes the server read, sort, and materialize the whole result before it sends row one, so the client's first fetch blocks through that entire window. An unsorted scan streams immediately and has almost no equivalent window. Parse-time errors were already caught by mysql_real_query's return code, which is why only this class slipped through silently.

2. The trigger. Since 0.55.0 the row cap was applied by appending LIMIT cap+1 to the SQL. That is not neutral: adding a LIMIT to an ORDER BY query moves MySQL 8.0 onto an ordered index scan (prefer_ordering_index) instead of a filesort. It is the one thing that differed between TablePro and the clients that worked, since they send the query as written.

What changed

  • The MySQL driver checks mysql_errno() on every exit path and throws the error the server reported, so a real failure shows up as an error instead of an empty table.
  • The row cap no longer rewrites the SQL. The query runs exactly as typed and the cap applies to the rows read back, which is how SQL Server and Oracle already behaved here, and what DataGrip and DBeaver do. SQLLimitInjector and the injectRowLimit driver hook are gone.
  • On truncation the driver issues KILL QUERY through the existing cancel path instead of draining every remaining row over the wire.
  • Fixed a separate bug: setting the row cap to unlimited (0) resolved to 0 rather than nil, so the over-fetch became LIMIT 1 and unlimited silently returned a single row.

Note for review

The injector did two jobs, and only one was wrong. It also detected an existing top-level LIMIT/FETCH/TOP and skipped the cap for that run. Deleting it outright would have capped an explicit LIMIT 50000 down to 10,000 and regressed #956 ("your own LIMIT wins"). The lexer is kept as a read only SQLLimitDetector; the string rewriting is what was unsound, not the scanning.

ABI

scripts/check-pluginkit-abi.sh main reports one change: injectRowLimit removed from PluginDatabaseDriver and its default extension. It defaulted to nil, which CLAUDE.md lists as additive, so no currentPluginKitVersion bump and no plugin re-release. Needs the abi-additive label.

Testing

  • swiftlint lint --strict: 0 violations.
  • New SQLLimitDetectorTests covering both reported repros, plus the ORDER BY coverage the old injector tests never had.
  • New resolveRowCap tests, including the unlimited-returns-one-row regression.
  • TableProPluginKit builds via SwiftPM after the protocol removal.

Not covered by the suite: the driver fix needs a live MySQL to exercise, since it is C-bridge code. Worth confirming on a real server that KILL QUERY on a truncated result surfaces ER_QUERY_INTERRUPTED (1317) and not a connection-lost code, otherwise every truncated query would report an error.

@datlechin datlechin added the abi-additive PluginKit ABI diff reviewed as additive; no version bump needed label Jul 16, 2026
@mintlify

mintlify Bot commented Jul 16, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Jul 16, 2026, 4:26 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@datlechin
datlechin merged commit d61be06 into main Jul 16, 2026
3 checks passed
@datlechin
datlechin deleted the fix/1884-mysql-order-by-empty-result branch July 16, 2026 16:28

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4e7e52580

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

do {
let fetchResult = try await queryExecutor.executeQuery(
sql: plan.executedSQL,
sql: sql,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce rowCap before executing default plugin queries

When a capped SELECT reaches a plugin that relies on PluginDatabaseDriver’s default executeUserQuery (for example PostgreSQL/ClickHouse/DuckDB-style .limit plugins), this now sends the original SQL and only passes rowCap; the default implementation calls execute(query:) first and trims raw.rows after the full result has already been materialized. Before this change, plan.executedSQL contained the injected LIMIT cap+1 for uncapped .limit dialect queries, so the row-cap setting bounded server/client work. Large uncapped result sets can now still be fully fetched and held in memory despite the row cap, so the affected drivers or default path need to stream/stop at rowCap before dropping the server-side limit.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abi-additive PluginKit ABI diff reviewed as additive; no version bump needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TablePro MySQL Query Sorting Exception Issue Submission Template

1 participant